Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test_runner: simplify test time tracking #52182

Closed
wants to merge 2 commits into from
Closed

test_runner: simplify test time tracking #52182

wants to merge 2 commits into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Mar 21, 2024

This PR simplifies some of the logic that has accumulated over time for tracking test durations.

test_runner: simplify test start time tracking

This commit simplifies the logic for tracking test start time.
The start time is now set only when a test/suite begins running.
If the test/suite never runs, a fallback is provided in postRun().

test_runner: simplify test end time tracking

This commit simplifies the logic for tracking test end time.
The end time is now only set in postRun(), which every test
runs when it ends.

@cjihrig
test_runner: simplify test start time tracking
f2e7bcc 
This commit simplifies the logic for tracking test start time.
The start time is now set only when a test/suite begins running.
If the test/suite never runs, a fallback is provided in postRun().
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Mar 21, 2024
@cjihrig cjihrig requested review from MoLow and atlowChemi March 22, 2024 12:05
@cjihrig cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 23, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 23, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/57917/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/57919/

@cjihrig cjihrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Mar 24, 2024
atlowChemi
atlowChemi approved these changes Mar 24, 2024
View reviewed changes
test/fixtures/test-runner/output/hooks_spec_reporter.snapshot Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I get why these changed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec reporter only shows times if they are non-zero. Those values were coming through as zeros before. I have now updated the PR as shown below. That seems to keep the snapshots the same, which can also save a second hrtime() call. Please take another look.

diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
index f61aada409..9f9c47edae 100644
--- a/lib/internal/test_runner/test.js
+++ b/lib/internal/test_runner/test.js
@@ -754,8 +754,8 @@ class Test extends AsyncResource {
   postRun(pendingSubtestsError) {
     // If the test was cancelled before it started, then the start and end
     // times need to be corrected.
-    this.startTime ??= hrtime();
     this.endTime ??= hrtime();
+    this.startTime ??= this.endTime;
 
     // The test has run, so recursively cancel any outstanding subtests and
     // mark this test as failed if any subtests failed.

@cjihrig
test_runner: simplify test end time tracking
7470065 
This commit simplifies the logic for tracking test end time.
The end time is now only set in postRun(), which every test
runs when it ends.
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/57938/

@cjihrig cjihrig added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Mar 25, 2024
@cjihrig cjihrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 25, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 25, 2024
@nodejs-github-bot
Copy link
Collaborator

Landed in a8b21fd...6af4049

nodejs-github-bot pushed a commit that referenced this pull request Mar 25, 2024
@cjihrig @nodejs-github-bot
test_runner: simplify test start time tracking
878047b 
This commit simplifies the logic for tracking test start time.
The start time is now set only when a test/suite begins running.
If the test/suite never runs, a fallback is provided in postRun().

PR-URL: #52182
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Mar 25, 2024
@cjihrig @nodejs-github-bot
test_runner: simplify test end time tracking
6af4049 
This commit simplifies the logic for tracking test end time.
The end time is now only set in postRun(), which every test
runs when it ends.

PR-URL: #52182
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
@cjihrig cjihrig deleted the test-times branch March 25, 2024 13:27
anonrig pushed a commit to anonrig/node that referenced this pull request Mar 25, 2024
@cjihrig @anonrig
test_runner: simplify test start time tracking
0af5aec 
This commit simplifies the logic for tracking test start time.
The start time is now set only when a test/suite begins running.
If the test/suite never runs, a fallback is provided in postRun().

PR-URL: nodejs#52182
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
anonrig pushed a commit to anonrig/node that referenced this pull request Mar 25, 2024
@cjihrig @anonrig
test_runner: simplify test end time tracking
9804b43 
This commit simplifies the logic for tracking test end time.
The end time is now only set in postRun(), which every test
runs when it ends.

PR-URL: nodejs#52182
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
MoLow
MoLow reviewed Mar 26, 2024
View reviewed changes
Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
@cjihrig @rdw-msft
test_runner: simplify test start time tracking
ba3b2d2 
This commit simplifies the logic for tracking test start time.
The start time is now set only when a test/suite begins running.
If the test/suite never runs, a fallback is provided in postRun().

PR-URL: nodejs#52182
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
@cjihrig @rdw-msft
test_runner: simplify test end time tracking
3d59acb 
This commit simplifies the logic for tracking test end time.
The end time is now only set in postRun(), which every test
runs when it ends.

PR-URL: nodejs#52182
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Apr 15, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
@cjihrig @marco-ippolito
test_runner: simplify test start time tracking
a4f7f4f 
This commit simplifies the logic for tracking test start time.
The start time is now set only when a test/suite begins running.
If the test/suite never runs, a fallback is provided in postRun().

PR-URL: #52182
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
@cjihrig @marco-ippolito
test_runner: simplify test end time tracking
f767918 
This commit simplifies the logic for tracking test end time.
The end time is now only set in postRun(), which every test
runs when it ends.

PR-URL: #52182
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
@marco-ippolito marco-ippolito mentioned this pull request May 2, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
@cjihrig @marco-ippolito
test_runner: simplify test start time tracking
07e4a42 
This commit simplifies the logic for tracking test start time.
The start time is now set only when a test/suite begins running.
If the test/suite never runs, a fallback is provided in postRun().

PR-URL: #52182
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
@cjihrig @marco-ippolito
test_runner: simplify test end time tracking
5cd3df8 
This commit simplifies the logic for tracking test end time.
The end time is now only set in postRun(), which every test
runs when it ends.

PR-URL: #52182
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MoLow MoLow MoLow left review comments

@atlowChemi atlowChemi atlowChemi approved these changes

@anonrig anonrig anonrig approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cjihrig @nodejs-github-bot @anonrig @MoLow @atlowChemi